feat: split allow_list_answers column in analytics_summary view #2926

Mike-Heneghan · 2024-03-26T13:04:38Z

What

Add separate columns for each of the possible ALLOW_LIST variables
Add the created_at for the analytics_log rather than just the created_at for the analytics session

Why

It was found that the allow_list_answers with the new data structure was difficult to work with on Metabase

Follow Up

Noticed that the allow_list_answer data structure is inconsistent i.e. {key: value} rather than {key: [value]}
Noticed that the null values are no longer being filtered out:

planx-new/editor.planx.uk/src/pages/FlowEditor/lib/analyticsProvider.tsx

Line 467 in 612fa87

const filteredAnswers = answerValues?.filter(Boolean);

which leads to [null] surprisingly often?
Check that the value of allow list as an array is usable on Metabase:
Remove the nodeFn column from analytics_logs as it's not reliable with the previous changes to the way we gather allow_list answers i.e. it naively stores the nodeToTrack?.data?.fn || nodeToTrack?.data?.val and missed when the passport variable is defined by the breadcrumbs.
Align the data structure for the allow_list between analytics_logs and lowcal_sessions i.e. [{key: [value]}], vs {key: [value]} respectively.

Mike-Heneghan · 2024-03-26T13:12:29Z

...11457331702_alter_view_public_analytics_summary_split_allow_list_answers_into_columns/up.sql

+	) as time_spent_on_analytics_session_minutes,
+	node_id,
+	al.allow_list_answers as allow_list_answers,
+    allow_list_answer_elements->>'proposal.projectType' AS proposal_project_type,


I was a little unsure about the format of the new column names.

I went for ~~camelcase~~ snake_case as I thought it was more consistent with the rest of the table but I'm happy to change to match the exact variable name if preferred?

I think keeping these as snake_case is correct as it's for a table / view. Metabase should automatically convert this to Proposal Project Type I think 👍

github-actions · 2024-03-26T13:29:51Z

Removed vultr server and associated DNS entries

DafyddLlyr

Fantastic - thanks for picking this up.

A few comments raised by PR -

[null] values

This should have been handled by this line https://github.com/theopensystemslab/planx-new/blob/main/editor.planx.uk/src/pages/FlowEditor/lib/analyticsProvider.tsx#L535 but maybe it's not working as intended?

{ key: value } vs { key: [value] }

Yep this is also a bit of an awkward one - values in arrays can contain multiples (e.g. project type) and strings are a single value (e.g. draw boundary option). I'm not sure how Metabase will handle these? We could concatenate arrays into a string (and sort them so they're consistent) but I would think an array is more usable? Not too sure here - maybe a question for @zz-hh-aa . We can format it however we want is the truth 👍

node_fn

Agreed - this column is probably redundant now we have allow list answers. I vote we remove it.

Align the data structure for the allow_list between analytics_logs and lowcal_sessions i.e. [{key: [value]}], vs {key: [value]} respectively.

Yep - we should do this!

DafyddLlyr · 2024-03-26T15:25:16Z

editor.planx.uk/src/pages/FlowEditor/lib/analyticsProvider.tsx

+/**
+ * If appending to ALLOW_LIST please also update the `analytics_summary` view to
+ * extract it into it's own column.
+ */


Perfect - I had this exact comment in mind also!

DafyddLlyr · 2024-03-26T15:26:31Z

...11457331702_alter_view_public_analytics_summary_split_allow_list_answers_into_columns/up.sql

+	) as time_spent_on_analytics_session_minutes,
+	node_id,
+	al.allow_list_answers as allow_list_answers,
+    allow_list_answer_elements->>'proposal.projectType' AS proposal_project_type,


I think keeping these as snake_case is correct as it's for a table / view. Metabase should automatically convert this to Proposal Project Type I think 👍

DafyddLlyr · 2024-03-26T15:47:03Z

...11457331702_alter_view_public_analytics_summary_split_allow_list_answers_into_columns/up.sql

+		) / 60 as numeric (10, 1)
+	) as time_spent_on_analytics_session_minutes,
+	node_id,
+	al.allow_list_answers as allow_list_answers,


I'm not sure we need to keep the current column - what do you think?

I don't think it adds much value beyond being a reference to check that data is being extracted properly etc.

I'm tempted to remove it 👀

Counter to my previous point I think it could also be handy to check whether a new value in ALLOW_LIST has been missed in the view 🤔

Yep this is a good enough reason for me to keep it - it can just not be used in Metabase directly 👍

DafyddLlyr · 2024-03-26T15:47:56Z

...11457331702_alter_view_public_analytics_summary_split_allow_list_answers_into_columns/up.sql

+	left join analytics_logs al on a.id = al.analytics_id
+	left join flows f on a.flow_id = f.id
+	left join teams t on t.id = f.team_id
+    left join lateral jsonb_array_elements(al.allow_list_answers) AS allow_list_answer_elements ON true;


Mike-Heneghan · 2024-03-26T16:40:57Z

Add column per allow_list_answer to the analytics_summary db view.

Mike-Heneghan · 2024-03-26T17:16:52Z

Fantastic - thanks for picking this up.

A few comments raised by PR -

[null] values

This should have been handled by this line https://github.com/theopensystemslab/planx-new/blob/main/editor.planx.uk/src/pages/FlowEditor/lib/analyticsProvider.tsx#L535 but maybe it's not working as intended?

{ key: value } vs { key: [value] }

Yep this is also a bit of an awkward one - values in arrays can contain multiples (e.g. project type) and strings are a single value (e.g. draw boundary option). I'm not sure how Metabase will handle these? We could concatenate arrays into a string (and sort them so they're consistent) but I would think an array is more usable? Not too sure here - maybe a question for @zz-hh-aa . We can format it however we want is the truth 👍

node_fn

Agreed - this column is probably redundant now we have allow list answers. I vote we remove it.

Align the data structure for the allow_list between analytics_logs and lowcal_sessions i.e. [{key: [value]}], vs {key: [value]} respectively.

Yep - we should do this!

Thanks for the thorough review and responses 🙌 @DafyddLlyr

Mike-Heneghan · 2024-03-26T17:17:14Z

[null] values

This should have been handled by this line https://github.com/theopensystemslab/planx-new/blob/main/editor.planx.uk/src/pages/FlowEditor/lib/analyticsProvider.tsx#L535 but maybe it's not working as intended?

Yeah you're right! I misread the updated code and missed this. It's an odd one but I double checked and these are new records so it's not a legacy issue it seems to be live in production at the moment. Also it's not specific to one allow_list_answer key: https://metabase.editor.planx.uk/question/353-null-allow-list-value

This might be worth splitting into it's own PR and including a data cleanup migration as I imagine the null values might be a pain in Metabase but we can check that after this I guess.

Mike-Heneghan · 2024-03-26T17:17:35Z

{ key: value } vs { key: [value] }

Yep this is also a bit of an awkward one - values in arrays can contain multiples (e.g. project type) and strings are a single value (e.g. draw boundary option). I'm not sure how Metabase will handle these? We could concatenate arrays into a string (and sort them so they're consistent) but I would think an array is more usable? Not too sure here - maybe a question for @zz-hh-aa . We can format it however we want is the truth 👍

I agree an array is probably more useful and Metabase might handle it gracefully 🤞 I incorrectly thought this was something to do with how we were writing the values to the logs rather than it being the data itself as below.

Taking that into account I reckon we maybe go ahead and see how we get on unless you have any objections @zz-hh-aa

Mike-Heneghan · 2024-03-26T17:18:02Z

node_fn
Agreed - this column is probably redundant now we have allow list answers. I vote we remove it.

Align the data structure for the allow_list between analytics_logs and lowcal_sessions i.e. [{key: [value]}], vs {key: [value]} respectively.
Yep - we should do this!

Nice one! I think I'll update both of these in a follow up PR as I think they're close enough logically in that the node_fn is moving from it's own column into the the JSONB allow_list_answers.

As part of it I think I'll also ditch the [] and make it a null in the same way that submission_services_summary has it: https://trello.com/c/zKgDcQAK/2860-update-analyticslogs-jsonb-default-values-to-null

That will also have to change how the db view is written 😅 Although the column names shouldn't change so I think it's still worth merging this to unblock but in the background change how we store the data 😌

zz-hh-aa · 2024-03-26T17:40:02Z

If I've understood correctly, I would vote in favour of strings--the Metabase GUI gets confused by JSONs but can handle strings, so it might be council-friendlier to go that route. This is a loosely held opinion!

If we go the strings route and later wanted to split the array items up, we could always separate by ,.

Mike-Heneghan · 2024-03-27T10:13:45Z

If I've understood correctly, I would vote in favour of strings--the Metabase GUI gets confused by JSONs but can handle strings, so it might be council-friendlier to go that route. This is a loosely held opinion!

If we go the strings route and later wanted to split the array items up, we could always separate by ,.

Good point @zz-hh-aa the change here would end up with the same data structure as input_errors which we had issues with I think? It would pick up the arrays but then when we tried to apply the filter we'd get error?

I'll have a look at updating this PR to make the column values a string separated by , if there are multiple variables as you suggest 👍

Mike-Heneghan · 2024-03-27T11:52:30Z

If I've understood correctly, I would vote in favour of strings--the Metabase GUI gets confused by JSONs but can handle strings, so it might be council-friendlier to go that route. This is a loosely held opinion!
If we go the strings route and later wanted to split the array items up, we could always separate by ,.

Good point @zz-hh-aa the change here would end up with the same data structure as input_errors which we had issues with I think? It would pick up the arrays but then when we tried to apply the filter we'd get error?
I'll have a look at updating this PR to make the column values a string separated by `,` if there are multiple variables as you suggest 👍

I tried updating this to turn the array into a string separated by columns with the view and it's surprisingly difficult 😬 I think it's because it's trying to do a lot of transformations at once and it's a bit of a nightmare.

I'd propose we make this change then handle it on Metabase with a "Saved Question" written in SQL which should hopefully be able to handle this?

zz-hh-aa · 2024-03-27T13:00:03Z

I'd propose we make this change then handle it on Metabase with a "Saved Question" written in SQL which should hopefully be able to handle this?

Yep that's a perfect workaround--thanks for looking into it!

Mike-Heneghan · 2024-03-27T16:24:47Z

Add column per allow_list_answer to the analytics_summary db view.

Mike-Heneghan · 2024-03-27T16:54:41Z

[null] values

This should have been handled by this line https://github.com/theopensystemslab/planx-new/blob/main/editor.planx.uk/src/pages/FlowEditor/lib/analyticsProvider.tsx#L535 but maybe it's not working as intended?

Yeah you're right! I misread the updated code and missed this. It's an odd one but I double checked and these are new records so it's not a legacy issue it seems to be live in production at the moment. Also it's not specific to one allow_list_answer key: https://metabase.editor.planx.uk/question/353-null-allow-list-value

This might be worth splitting into it's own PR and including a data cleanup migration as I imagine the null values might be a pain in Metabase but we can check that after this I guess.

I think the issue was that https://github.com/theopensystemslab/planx-new/blob/main/editor.planx.uk/src/pages/FlowEditor/lib/analyticsProvider.tsx#L535 handles answers returned from getData but not from getAnswers

Mike-Heneghan self-assigned this Mar 26, 2024

Mike-Heneghan force-pushed the mh/allow-list-answers-columns-analytics-summary branch from 642d69e to c30abd2 Compare March 26, 2024 13:08

Mike-Heneghan added 2 commits March 26, 2024 13:10

feat: split allow_list_answers column in analytics_summary view

2b77b60

chore: add comment about updating the view when updating the ALLOW_LIST

e375827

Mike-Heneghan force-pushed the mh/allow-list-answers-columns-analytics-summary branch from c30abd2 to e375827 Compare March 26, 2024 13:10

Mike-Heneghan commented Mar 26, 2024

View reviewed changes

Mike-Heneghan marked this pull request as ready for review March 26, 2024 13:16

Mike-Heneghan requested review from a team and zz-hh-aa March 26, 2024 13:18

DafyddLlyr approved these changes Mar 26, 2024

View reviewed changes

Mike-Heneghan mentioned this pull request Mar 26, 2024

feat: split submission_services_summary allow_list_answers into their own columns in view #2927

Merged

Mike-Heneghan merged commit 2b2afb2 into main Mar 27, 2024
12 checks passed

Mike-Heneghan deleted the mh/allow-list-answers-columns-analytics-summary branch March 27, 2024 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: split allow_list_answers column in analytics_summary view #2926

feat: split allow_list_answers column in analytics_summary view #2926

Mike-Heneghan commented Mar 26, 2024 •

edited

Loading

Mike-Heneghan Mar 26, 2024 •

edited

Loading

DafyddLlyr Mar 26, 2024

github-actions bot commented Mar 26, 2024 •

edited

Loading

DafyddLlyr left a comment

DafyddLlyr Mar 26, 2024

DafyddLlyr Mar 26, 2024

DafyddLlyr Mar 26, 2024

Mike-Heneghan Mar 26, 2024 •

edited

Loading

Mike-Heneghan Mar 26, 2024

DafyddLlyr Mar 26, 2024

DafyddLlyr Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

zz-hh-aa commented Mar 26, 2024

Mike-Heneghan commented Mar 27, 2024

Mike-Heneghan commented Mar 27, 2024

zz-hh-aa commented Mar 27, 2024

Mike-Heneghan commented Mar 27, 2024

Mike-Heneghan commented Mar 27, 2024

feat: split allow_list_answers column in analytics_summary view #2926

feat: split allow_list_answers column in analytics_summary view #2926

Conversation

Mike-Heneghan commented Mar 26, 2024 • edited Loading

What

Why

Follow Up

Mike-Heneghan Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

DafyddLlyr Mar 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 26, 2024 • edited Loading

DafyddLlyr left a comment

Choose a reason for hiding this comment

DafyddLlyr Mar 26, 2024

Choose a reason for hiding this comment

DafyddLlyr Mar 26, 2024

Choose a reason for hiding this comment

DafyddLlyr Mar 26, 2024

Choose a reason for hiding this comment

Mike-Heneghan Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Mike-Heneghan Mar 26, 2024

Choose a reason for hiding this comment

DafyddLlyr Mar 26, 2024

Choose a reason for hiding this comment

DafyddLlyr Mar 26, 2024

Choose a reason for hiding this comment

Mike-Heneghan commented Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

Mike-Heneghan commented Mar 26, 2024

zz-hh-aa commented Mar 26, 2024

Mike-Heneghan commented Mar 27, 2024

Mike-Heneghan commented Mar 27, 2024

zz-hh-aa commented Mar 27, 2024

Mike-Heneghan commented Mar 27, 2024

Mike-Heneghan commented Mar 27, 2024

Mike-Heneghan commented Mar 26, 2024 •

edited

Loading

Mike-Heneghan Mar 26, 2024 •

edited

Loading

github-actions bot commented Mar 26, 2024 •

edited

Loading

Mike-Heneghan Mar 26, 2024 •

edited

Loading